Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support mapping env vars to multiple settings #189

Merged
merged 1 commit into from
Mar 30, 2017

Conversation

twitchard
Copy link
Contributor

Addresses issue #124

I didn't add an overridable option to toggle the old behavior and throw on envvar reuse. My argument is as follows:

  1. I throught it would be confusing for a schema to load successfully, but then to start throwing errors about an invalid schema on the call to .validate().
  2. I thought that adding a global, setting-independent optional parameter to the function call that loads the schema convict(...) would add too much complexity, because no such optional parameter already exists.
  3. This allows for most user flexibility. If the user still wants to forbid multiple environment variables, they may write their own function to do it and pass their schema through it before initializing convict. Whereas, if the user wants to permit mapping env vars to multiple settings, it is impossible with the current behavior.
  4. This is not a breaking change. This will not change any schemas which were considered valid.

If you're not persuaded, I would be happy to try a different approach along the lines of #2 in my list if you think that is best. Otherwise, I've also made a PR #188 that documents and adds a test for the current behavior.

@coveralls
Copy link

coveralls commented Mar 18, 2017

Coverage Status

Coverage decreased (-0.3%) to 92.754% when pulling 03eb43a on twitchard:env-var-multiple-settings into 78d9621 on mozilla:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 92.754% when pulling 03eb43a on twitchard:env-var-multiple-settings into 78d9621 on mozilla:master.

@madarche
Copy link
Collaborator

Hello @twitchard. I really do appreciate the way you have prepared the 2 competing PRs. Thanks! For now I have accepted #188.

What do other interested parties think about this PR please?

@madarche
Copy link
Collaborator

I'm convinced by @twitchard' rationale. And I'm for accepting this PR. I plan to merge it just after #191 lands.

@madarche
Copy link
Collaborator

madarche commented Mar 30, 2017

@twitchard could you rebase your commit and resolve the conflicts please? I would be very happy to then merge your PR. Thank you!

@twitchard
Copy link
Contributor Author

Done!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 29ed406 on twitchard:env-var-multiple-settings into 7dbb2a7 on mozilla:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 29ed406 on twitchard:env-var-multiple-settings into 7dbb2a7 on mozilla:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 29ed406 on twitchard:env-var-multiple-settings into 7dbb2a7 on mozilla:master.

@madarche
Copy link
Collaborator

You're so fast guys 😄

Thanks @twitchard!

@madarche madarche merged commit 1d2b577 into mozilla:master Mar 30, 2017
@twitchard twitchard deleted the env-var-multiple-settings branch March 30, 2017 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants